feat: make the worker handoff write path cancellable - #1237

Correct. Fixed in 822f166c.

Deriving the deletion timeout from the signal context is what created it: before this branch removeImages built its own budget from context.Background(), so cancellation could not reach it and could not be misreported by it. Once it can, nil from removeImages stops meaning "the images are gone".

main now refuses to treat the removal as successful without checking:

// A signal that landed during removal was consumed rather than killing the
// process, and with --imagelist there is no completion write below to report
// it, so an interrupted run would otherwise exit 0 having removed nothing.
if err := ctx.Err(); err != nil {
    log.Error(err, "terminating before removal finished", "removed", removed)
    os.Exit(generalErr)
}

I took your fallback rather than the first suggestion deliberately, because the two are at different layers and this PR only owns one of them. Returning the context error from inside the delete branches also stops the loop early, which is a behavior change to the removal loop itself; the stacked #1239 makes it, with the tests for it, because that PR is what gives each deletion its own budget and therefore has to distinguish "this image ran out of time, carry on" from "the caller is gone, stop". Here the only defect is the exit status, so that is all that changes: an interrupted run still walks the remaining list logging instant failures, but it can no longer exit 0 while doing it.

Right, and it inverts the point of the PR. Fixed in 822f166c.

The read is only interruptible while the endpoint is missing. Once the peer publishes it, os.OpenFile blocks in the kernel on the FIFO, and on Windows the watcher closes the listener — which does nothing for a connection already accepted and sitting in io.ReadAll. So a peer that published and then stalled left the remover ignoring SIGTERM until the kubelet's SIGKILL, where before this branch it died immediately. A PR whose stated purpose is "a terminating pod shouldn't wait for the kill" made that case strictly worse.

Taking your first option — registration now starts after the read:

// Registering the handler suppresses the default SIGTERM exit, so it starts
// only here: once the peer publishes its endpoint the read above blocks in a
// call no context can interrupt, and covering it would swallow the signal
// until SIGKILL. The stop func is discarded rather than deferred because
// every exit path below is os.Exit, which would skip it anyway.
ctx, _ := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)

Everything after that point — removeImages, the metrics calls, both WriteCompletionPipe calls — takes ctx and observes it, so the handler covers exactly the calls that can act on it. That is also where main had it before this branch; moving it to the top was my own overreach and nothing required it.

Not the second option, at least not here. Making the read genuinely cancellable means giving up the blocking FIFO open, and that open is the rendezvous every existing deployment depends on — the same trade I argued against for the write side in the description, with the same problem that I cannot execute the Unix path locally to check it. The default disposition is also better than partial cancellation: it terminates immediately, with none of my code in the path.

Worth noting the collector already had the rule written down, and I simply failed to apply it to the remover:

// Registering the handler suppresses the default SIGTERM exit, so it covers
// exactly the one call that observes ctx. Everything above builds its own
// timeouts from Background, and Await below has no context at all; holding
// the handler across either would swallow the signal.

Correct — the coverage was theatre. Fixed in 768d5bfc.

Every case in TestRemoveImages passes a context that is never done, and the fake discarded the argument outright, so reverting context.WithTimeout(ctx, timeout) to context.WithTimeout(context.Background(), timeout) left the suite green. That is the one thing this hunk exists to do.

The fake now observes its context, which is what a real client does anyway, and the propagation is pinned by behavior rather than by inspection:

func TestRemoveImagesPassesTheCallersContextToTheRuntime(t *testing.T) {
	ctx, cancel := context.WithCancel(context.Background())
	cancel()

	client := &testClient{t: t, images: []*v1.Image{{Id: "sha256:aaaa"}}}

	removed, err := removeImages(ctx, client, []string{"sha256:aaaa"})
	...
}

Against context.Background() it reports removed = 1, want 0 and the image was deleted for a caller that was already gone.

I went with an already-cancelled caller rather than the blocking fake you suggested, because a blocking fake asserts that cancellation unblocks removeImages, and here nothing blocks — DeleteImage is a call, not a wait, and the loop has no guard in this PR. The question this hunk raises is narrower: does the runtime see the caller's context or a detached one. An already-dead context answers exactly that, without inventing a blocking behavior the real CRI client doesn't have.

Worth flagging for when you look at the stacked #1239: it adds a guard that returns before the loop reaches the runtime at all, which makes this test assert the wrong thing there, so it is removed in the commit that introduces the guard. The propagation stays covered there by TestRemoveImagesSurfacesCancellationDuringTheFinalDeletion, which cancels during a deletion and so still requires the runtime to be holding the caller's context.

The race is real and I said so when I added the probe, but I don't think it reaches the impact described, so I'd rather argue it than implement handle-based deletion on a guess.

Two things bound it:

The path is a compile-time constant. There is no traversal component and nothing attacker-influenced in it, so the entry that gets deleted is always inside the shared volume.

os.Remove does not follow reparse points. It is DeleteFileW then RemoveDirectoryW, both of which act on the link rather than its target. Swapping the socket for a symlink or junction gets the link deleted, not whatever it points at — so this cannot be turned into "make SYSTEM delete a file outside the volume", which is the version that would actually matter.

What's left is that a scanner which wins a sub-millisecond race can cause SYSTEM to delete a file in a directory the scanner already has write access to, and could therefore delete itself. That is a correctness wart, not a privilege boundary: the stated guarantee is weaker than I implied, but nothing crosses a trust boundary when it fails.

That said, the guarantee is weaker than advertised, and I'd rather the code say what it does. Two options, and I'm happy with either:

1. Narrow the claim in the comment to what the check actually provides — a same-process-view guard against replacing a non-socket, not an atomic one.
2. Do the identity-checked delete: open with FILE_FLAG_OPEN_REPARSE_POINT, confirm the tag is IO_REPARSE_TAG_AF_UNIX through GetFileInformationByHandleEx, then delete via FileDispositionInfo on that handle.

I'd want to do (2) on a real Windows node rather than from reasoning about the API, since it is x/sys/windows surface with no coverage in this repo today, and it is orthogonal to what this PR is about. If you'd like it, I'd rather it were its own PR with its own validation. Tell me which you prefer and I'll do it.

This is resolved rather than narrowed now, and not by the handle-based delete I offered.

listen no longer deletes anything at all — the reclaim it existed to serve was defending a case that cannot occur, because the shared volume is an emptyDir created with the pod and restartPolicy is Never, so no retry inherits a socket. Details in b833fd95 and the thread on line 243.

With no os.Remove, there is no Lstat/Remove window to race. My earlier answer argued the residual race had no privilege impact; that argument is now moot, which is a better outcome than being right about it.

Right, and it's a defect I introduced last round rather than one that was already there. Fixed in 080a917a.

The probe is a connection, and these endpoints serve exactly one. So listen would correctly decline to unlink a live socket, and the listener it had just protected would accept the probe, read zero bytes, and be left with nothing still to come — the worker stranded either way, only now with a confusing empty payload instead of a missing endpoint.

Await and ReadImagesPipe now keep accepting until something actually says something:

data, err := io.ReadAll(conn)
_ = conn.Close()
if err != nil {
    return nil, err
}

// A connect that says nothing is not the peer [...]
if len(data) == 0 {
    continue
}

I took the accept-side option rather than "avoid connecting", because I don't think a non-connecting liveness check exists here that is worth the complexity: the mode is identical, the bind fails either way, and anything file-based reintroduces its own staleness question. The accept side is also where this belonged regardless of the probe — the volume is shared with a scanner image we don't control, so the peer was never the only thing that could connect, and a single stray connect could already have consumed the handoff before this PR existed. Distinguishing by payload is what makes that safe, and nothing legitimate sends an empty one: the image list is JSON, the completion message is a constant.

TestAwaitIgnoresAConnectThatSaysNothing queues a connect ahead of the peer exactly the way listen does, against a listener that is already published so the ordering is deterministic. Without the skip it fails with:

payload = "", want "complete" -- the probe was taken for the peer

Following up: the probe this thread was about is gone entirely as of b833fd95 — a later review round showed that a failed probe doesn't prove staleness either (a live listener with a full backlog refuses), and the reclaim it served was defending a case that cannot occur. See the thread on line 243.

The accept-side fix from this thread stays, and it's worth saying why now that its original motivation has been deleted: the volume is shared with a scanner image we don't control, so the peer was never the only thing that could connect. A stray connect could consume the handoff independently of anything listen did. TestAwaitIgnoresAConnectThatSaysNothing keeps that pinned; only its comment changed, to stop citing the probe as the reason.

Could an empty connection return a protocol error instead of waiting for another connection? listen no longer creates probe connections, so an empty read now means the writer exited or was canceled before sending. Since it will not reconnect, could this leave the peer blocked until job cleanup?

Good catch and the justification for that loop expired earlier in this PR without my noticing, fixed now

Could SIGTERM be lost between this ctx.Err() call and stopSignals()? If it arrives in that window, notification can consume it after sigErr captured nil, and the collector then enters uncancellable Await(). Could we remove that race or make the wait cancellable?

Sure we can, updated to remove the race.

On the second option, I think making Await take a context is the better state and. However, I'd like to do in a follow up pr if that is okay to you because it changes the handoff API on both platforms, and on the Unix side the read carries the same partial-cancellation caveat as the write

Is this intended to support every !windows target or only Linux? On Darwin, closing the file here does not wake the blocked Write, and the added test fails after 30 seconds. Should this use nonblocking I/O or be scoped to supported platforms?

Thanks for actually running it on Darwin — I couldn't have caught that from here. Scoped in 247f10e.

The honest answer to "every !windows target or only Linux" is: the rendezvous half of cancellation works anywhere, and the mid-write half is Linux-only. Closing a FIFO wakes a blocked write on Linux; it doesn't on Darwin, so the test sat there for its full 30 seconds asserting something the platform doesn't provide.

I've kept the file at !windows rather than narrowing to linux, because restricting it would stop the package building on a developer's Mac for no gain — the code is correct there, just weaker. The comment now says which half is guaranteed where, and the test skips off Linux instead of quietly failing:

if runtime.GOOS != "linux" {
    t.Skipf("closing a blocked FIFO write is only guaranteed to unblock it on linux, not %s", runtime.GOOS)
}

I'd rather not reach for nonblocking I/O to make it uniform. That means giving up the blocking open that is the rendezvous every existing deployment depends on, for a platform Eraser's workers don't run on — the nodes are Linux and Windows. If that trade looks wrong to you I'll take another run at it, but it felt like a lot of risk to buy Darwin parity in a test.

Could cancellation also terminate this blocked open? The caller returns, but this goroutine remains in OpenFile until a reader arrives, at which point that reader receives EOF from the abandoned opener. Would a nonblocking open loop avoid the goroutine and OS-thread leak?

The leak is real and I'm not going to claim otherwise — but I'd like to argue for leaving it, because I think the consequence is smaller than it looks and the fix is riskier than it looks.

Why it's bounded. Every caller of this is a worker whose next move after a cancelled write is os.Exit. The goroutine and its OS thread live until the process does, which is milliseconds later. There's no accumulation: it is one goroutine, once, at the end of a run that is already terminating.

On the spurious EOF. That needs a reader to arrive at the same endpoint after the writer gave up. Within a pod there is exactly one reader per endpoint, and it is either already blocked in open — in which case the rendezvous completed and this path was never taken — or it never arrives at all. Across pods it can't happen: the volume is an emptyDir created with the pod, and restartPolicy is Never, so a worker that dies is replaced by a new pod with a new volume rather than restarted onto this one.

Why I'm wary of the nonblocking loop. O_WRONLY|O_NONBLOCK on a FIFO returns ENXIO until a reader is present, so a poll loop is semantically equivalent and genuinely cancellable — you're right that it would work. What it changes is the syscall every existing Linux deployment currently rendezvouses on, in a PR that is already eight commits deep, to remove a goroutine that outlives its process by milliseconds. I've argued against that trade twice in this PR for the payload write and I don't think it gets better here.

Where I'd change my mind: if Await grows a context — which came up on the collector thread too — the read side would need the same treatment, and at that point doing both together in one focused PR is clearly better than doing this half here. I'd rather that than bolt it on now.

Happy to do it either way if you'd prefer it in this PR; I just don't want to make that call unilaterally.

Should context.Canceled stop the deletion loop immediately? At present it is handled like an ordinary per-image error, so every remaining DeleteImage call is attempted before main notices cancellation. Could we return the context error here and in the prune loop?

Agreed, and it exists — in the stacked #1239, with tests. Two commits there do exactly what you describe:

• 085dc1c adds the guard to the top of both loops, so a cancelled caller stops rather than walking the rest of the list
• 588635d handles the case the guard can't see: cancellation during the last deletion, where there is no later iteration to reach the guard

The split was deliberate but I'm now the only one who thinks so, which is usually the sign it was wrong. My reasoning was that returning the context error from inside the delete branches changes the loop's behaviour, and #1239 is the PR that has to distinguish "this image ran out of its own budget, carry on" from "the caller is gone, stop" — because it's the one that gives each deletion a budget. Here, without per-image budgets, the only defect is the exit status, so that's all I changed: main checks ctx.Err() after removeImages and exits non-zero, which stops an interrupted run reporting success even though it still walks the remaining list.

You're the second reviewer to raise it against this PR, so if you'd rather #1237 not merge without the early stop, say so and I'll move both hunks down here and rebase #1239 on top. It's a contained change; I just didn't want to duplicate it across two open PRs on my own initiative.

Could this test enforce the deadline around ReadImagesPipe itself? Once the FIFO opens or the socket accepts, ReadAll does not observe ctx, so this call can still hang until the package timeout. Would running it in a goroutine and selecting on the deadline make the bound effective?

Good catch and the deadline was only binding on the write half, fixed.

Worth noting the underlying point survives the test fix: the read genuinely is uncancellable past the rendezvous. The test now fails instead of hanging, but that is just a diagnosis improvement, the real answer is Await and the read path taking a context, which is the thread on the collector.